api: expose root Stats::Scope via Api::Api by ahedberg · Pull Request #6712 · envoyproxy/envoy

ahedberg · 2019-04-25T20:31:38Z

Description: Add rootScope() to Api::Api so stats can be read by filters, etc. without making an admin request.
Risk Level: Low
Testing: bazel test //test/...
Docs Changes: N/A
Release Notes: N/A (but happy to add if warranted)
Fixes #6706

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

This reverts commit 2728a65. Signed-off-by: Ashley Hedberg <ahedberg@google.com>

mattklein123

Thanks, LGTM. Is it possible to add a test somewhere that actually uses this in a similar way to what you are going to do in your private filter? We have integration test filters now so it might be easy enough to do there. Thank you!

/wait

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

ahedberg · 2019-05-02T20:59:08Z

Still working on tests, but wanted to throw up the APIs for early feedback.

mattklein123 · 2019-05-02T21:31:11Z

Still working on tests, but wanted to throw up the APIs for early feedback.

Looks great, thank you.

/wait

eds_ready_filter.cc) Signed-off-by: Ashley Hedberg <ahedberg@google.com>

source/common/stats/thread_local_store.cc

it through the rootScope. Signed-off-by: Ashley Hedberg <ahedberg@google.com>

ahedberg · 2019-05-09T15:36:09Z

@jmarantz - any idea why the new eds_ready_filter can't find the stat in the root scope? Some very hacky debug logging suggests that the "cluster.cluster_0.membership_healthy" stat is added to scopes whose parent is the root scope, but querying the root scope for that stat returns nothing.

include/envoy/stats/scope.h

source/common/stats/isolated_store_impl.h

source/common/stats/thread_local_store.cc

test/integration/eds_integration_test.cc

find* Signed-off-by: Ashley Hedberg <ahedberg@google.com>

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

ahedberg · 2019-05-14T15:43:42Z

We have an integration test!

I think the main outstanding questions here are:

How to handle prefixes
- Folks querying the root scope will (presumably) specify a fully-qualified name, and don't want prefixes appended.
- Folks querying child scopes might expect that the prefix will be added.
How to handle lookups when there are overlapping scopes

jmarantz

This looks basically ready to go; just one simplification comment and a nit or two.

source/common/stats/isolated_store_impl.h

source/common/stats/thread_local_store.cc

jmarantz · 2019-05-15T00:58:25Z

source/common/stats/thread_local_store.cc

+  }
+
+  std::shared_ptr<StatType>* central_ref = &(iter->second);
+  if (tls_cache) {


I think you indicated 'done' in the comment I made earlier suggesting we not bother with the tls cache, since you are holding a lock anyway. I see you are no longer reading from it, but I'm not sure that usage of the 'find' method is a good signal that we should populate the current thread's cache. In particular -- not really fully understanding the application -- I was wondering if this might tend to be run from the main thread, whose tls cache will (I believe) be consulted approximately never. @mattklein123 & @fredlas for opinions on this.

Ah, I see--I think I misunderstood a previous conversation that we should populate the tls cache upon reading. I don't feel strongly either way.

We're planning on calling this from a filter, similar to eds_ready_filter.cc, if that changes anyone else's opinions.

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

jmarantz

Thanks @ahedberg -- looks great. I am in favor of this simpler approach since you are holding the lock anyway, the tls cache doesn't add value for this flow.

And the confusion you had earlier was my fault -- I had mistakenly suggested you populate the tls cache because I did not realize you had to hold the lock anyway to loop over scopes_.

source/common/stats/thread_local_store.h

mattklein123

Thanks looks awesome. 1 question and comment.

/wait

source/common/stats/thread_local_store.h

source/common/stats/thread_local_store.cc

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

mattklein123

Thank you!

mattklein123 · 2019-05-16T02:53:40Z

@ahedberg I'm pretty confused about how the OSX build is failing and not the others, but the failure looks legit. Can you take a look?

/wait-any

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

ahedberg · 2019-05-16T13:41:20Z

Switched to the version of sendLocalReply accepting response details, both because it's not deprecated and because MacOS doesn't seem to realize the other one exists. ¯_(ツ)_/¯

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

mattklein123

I suspect that Azure pipelines is actually merging master before it runs CI which is kind of interesting, and that's why it caught this error. @lizan is that true? Is that expected?

ahedberg added 3 commits April 25, 2019 14:12

Fix router_upstream_log_test

2728a65

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

Add rootScope() member function to Api::Api.

03442f8

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

Revert "Fix router_upstream_log_test"

4887f8f

This reverts commit 2728a65. Signed-off-by: Ashley Hedberg <ahedberg@google.com>

mattklein123 self-assigned this Apr 26, 2019

mattklein123 requested changes Apr 26, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 26, 2019

ahedberg mentioned this pull request Apr 26, 2019

Expose stats store singleton via Api::Api #6706

Closed

ahedberg added 2 commits May 2, 2019 09:20

Merge remote-tracking branch 'upstream/master' into expose_root_scope

76025c1

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

Add getCounter, getGauge, getName to Scope

4c1762d

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

repokitteh-read-only bot removed the waiting label May 2, 2019

repokitteh-read-only bot added the waiting label May 2, 2019

Working towards EDS integration test (still broken, see TODO in

901c211

eds_ready_filter.cc) Signed-off-by: Ashley Hedberg <ahedberg@google.com>

mattklein123 mentioned this pull request May 7, 2019

stats: prevent unused counters from leaking across hot restart #6850

Merged

fredlas reviewed May 8, 2019

View reviewed changes

source/common/stats/thread_local_store.cc Outdated Show resolved Hide resolved

ahedberg changed the title ~~api: expose root Stats::Scope via Api::Api~~ [WIP] api: expose root Stats::Scope via Api::Api May 9, 2019

Add debug logging to figure out why the stat doesn't exist when reading

75737ec

it through the rootScope. Signed-off-by: Ashley Hedberg <ahedberg@google.com>

repokitteh-read-only bot removed the waiting label May 9, 2019

mattklein123 added the waiting label May 9, 2019

jmarantz reviewed May 10, 2019

View reviewed changes

jmarantz self-assigned this May 10, 2019

ahedberg added 2 commits May 13, 2019 11:39

Change to absl::optional<std::reference_wrapper<const Stat>>, rename to

b869872

find* Signed-off-by: Ashley Hedberg <ahedberg@google.com>

Add ThreadLocalStore tests for find* methods

f4fd945

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

repokitteh-read-only bot removed the waiting label May 13, 2019

mattklein123 added the waiting label May 13, 2019

ahedberg added 3 commits May 14, 2019 11:29

Integration test works, clean up some errant log lines

48e97ee

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

Merge remote-tracking branch 'upstream/master' into expose_root_scope

1b8d010

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

Fix test after merge

22e9269

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

repokitteh-read-only bot removed the waiting label May 14, 2019

jmarantz reviewed May 15, 2019

View reviewed changes

Clean up comments, make IsolatedStatsCache::find private.

b95f77e

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

ahedberg changed the title ~~[WIP] api: expose root Stats::Scope via Api::Api~~ api: expose root Stats::Scope via Api::Api May 15, 2019

Don't update TLS cache on findStat

ee59b8f

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

jmarantz previously approved these changes May 15, 2019

View reviewed changes

source/common/stats/thread_local_store.h Outdated Show resolved Hide resolved

mattklein123 requested changes May 15, 2019

View reviewed changes

source/common/stats/thread_local_store.h Outdated Show resolved Hide resolved

source/common/stats/thread_local_store.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label May 15, 2019

s/findStat/findStatLockHeld, remove early rejects

bfe397a

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

ahedberg dismissed jmarantz’s stale review via bfe397a May 15, 2019 23:48

repokitteh-read-only bot removed the waiting label May 15, 2019

jmarantz previously approved these changes May 15, 2019

View reviewed changes

mattklein123 previously approved these changes May 16, 2019

View reviewed changes

repokitteh-read-only bot added the waiting:any label May 16, 2019

Use five-arg version of sendLocalReply to appease MacOS

e369e30

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

ahedberg dismissed stale reviews from mattklein123 and jmarantz via e369e30 May 16, 2019 13:39

repokitteh-read-only bot removed the waiting:any label May 16, 2019

Merge remote-tracking branch 'upstream/master' into expose_root_scope

a145485

Signed-off-by: Ashley Hedberg <ahedberg@google.com>

jmarantz approved these changes May 16, 2019

View reviewed changes

mattklein123 approved these changes May 16, 2019

View reviewed changes

mattklein123 merged commit 4c80e73 into envoyproxy:master May 16, 2019

Conversation

ahedberg commented Apr 25, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

ahedberg commented May 2, 2019

Uh oh!

mattklein123 commented May 2, 2019

Uh oh!

Uh oh!

ahedberg commented May 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahedberg commented May 14, 2019

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmarantz May 15, 2019

Choose a reason for hiding this comment

Uh oh!

ahedberg May 15, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented May 16, 2019

Uh oh!

ahedberg commented May 16, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants